[DRAFT] Use global.json to restrict .NET SDK use #3797
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces global.json to standardize the .NET SDK version (9.0.0) across the project and migrates pipeline files to use UseDotNet@2 with useGlobalJson: true instead of hardcoded version numbers or the ensure-dotnet-version.yml template. The changes simplify SDK installation, remove unnecessary runtime installations in build-only scenarios, and reduce redundant .NET Framework test targets in the stress tests.
Key changes:
- Added
global.jsonspecifying .NET 9.0 SDK withrollForward: "feature"policy - Updated 6 pipeline files to use
UseDotNet@2withuseGlobalJson: true - Removed installation of .NET 8.x runtime in build-only scenarios where tests aren't run
- Reduced stress test .NET Framework targets from 6 versions to just net462
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| global.json | New file specifying .NET 9.0.0 SDK requirement with feature rollforward and no prerelease versions |
| eng/pipelines/steps/compound-build-akv-step.yml | Migrated from hardcoded SDK/runtime installation to global.json; removed unnecessary .NET 8.x runtime |
| eng/pipelines/stages/stress-tests-ci-stage.yml | Reduced .NET Framework test targets from 6 versions to just net462 |
| eng/pipelines/jobs/stress-tests-ci-job.yml | Updated to use global.json for SDK installation and changed .NET 8 runtime from '8.x' to '8.0' |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Added net9.0 to x86 test target frameworks with explanatory comments |
| eng/pipelines/common/templates/steps/build-all-configurations-signed-dlls-step.yml | Migrated to global.json; removed unnecessary .NET 8.x runtime installation |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Simplified x86 SDK/runtime installation using global.json and added .NET 8.0 runtime for tests |
| eng/pipelines/common/templates/jobs/ci-code-coverage-job.yml | Replaced ensure-dotnet-version template with UseDotNet@2 using global.json |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| parameters: | ||
| packageType: 'sdk' | ||
| version: '10.0' | ||
| # Install whichever .NET SDKs are specified in our global.json. |
There was a problem hiding this comment.
Code coverage only runs on Windows x64, so we don't need the special ensure-dotnet.version template here.
| packageType: sdk | ||
| version: '9.x' | ||
|
|
||
| - task: UseDotNet@2 |
There was a problem hiding this comment.
We don't need a .NET 8 runtime to build things. We would only need it to run tests, which doesn't happen as part of this pipeline tree.
| packageType: 'sdk' | ||
| version: '9.x' | ||
|
|
||
| - task: UseDotNet@2 |
There was a problem hiding this comment.
We don't need a .NET 8 runtime to build things. We would only need it to run tests, which doesn't happen as part of this pipeline tree.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
eng/pipelines/jobs/stress-tests-ci-job.yml:146
- With the stress tests now targeting net9.0 (as specified in netTestRuntimes parameter which defaults to [net8.0, net9.0]), the .NET 9.0 runtime should also be installed. Consider adding:
# Install the .NET 9.0 runtime to run tests that target it.
- task: UseDotNet@2
displayName: Install .NET 9.0 Runtime
inputs:
packageType: runtime
version: 9.x # Install the .NET 8.0 runtime to run tests that target it.
- task: UseDotNet@2
displayName: Install .NET 8.0 Runtime
inputs:
packageType: runtime
version: 8.x
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 16 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| - name: timeout | ||
| type: number | ||
|
|
||
| # True if this is an ARM64 job. |
There was a problem hiding this comment.
We don't need to detect ARM64 - we already know explicitly which agent images are ARM64 and we can simply set this flag in those cases.
| downloadedNugetPath: $(Pipeline.Workspace)\${{parameters.artifactName }} | ||
| debug: ${{ parameters.debug }} | ||
|
|
||
| - ${{ else }}: # project |
There was a problem hiding this comment.
It was weird for the pre-build step to run a build. This has been moved out to the calling template, along with the installation of the SDKs and Runtimes.
| - allNoDocs | ||
|
|
||
| steps: | ||
| - template: ./ensure-dotnet-version.yml@self |
There was a problem hiding this comment.
This now happens earlier in the template tree.
| images: # list of images to run tests on | ||
| Win22_Sql19_x86: ADO-MMS22-SQL19 # stage display name: image name from the pool | ||
| TargetFrameworks: [net8.0] #[net462, net8.0] # list of target frameworks to run | ||
| TargetFrameworks: [net462, net8.0, net9.0] |
There was a problem hiding this comment.
Why not test all of the runtimes we support?
| pool: ADO-CI-PUBLIC-ARM64-1ES-EUS-POOL | ||
| images: | ||
| Win22_Azure_ARM64_Sql: ADO-WIN11-ARM64 | ||
| architecture: arm64 |
There was a problem hiding this comment.
This is the only agent image that is ARM64, so we can simply tell the downstream templates explicitly rather than trying to detect agent OS and architecture.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
eng/pipelines/jobs/stress-tests-ci-job.yml:145
- The stress test job installs the .NET 8.0 runtime but not the .NET 9.0 runtime. Since the stress tests target net9.0 (as seen in stress-tests-ci-stage.yml line 57 which defaults to
[net8.0, net9.0]), the .NET 9.0 runtime should also be installed.
Add a step after line 144 to install the .NET 9.0 runtime:
- task: UseDotNet@2
displayName: Install .NET 9.0 Runtime
inputs:
packageType: runtime
version: 9.x - task: UseDotNet@2
displayName: Install .NET 8.0 Runtime
inputs:
packageType: runtime
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
paulmedynski
force-pushed
the
dev/paul/global-json
branch
from
1388f65 to
bf917e9
Compare
…on, currently 9.0. - Using global.json to install .NET SDKs everywhere except when we may need ARM64. - Removed unnecessary installation of .NET SDKs and runtimes where they aren't needed. - Added all target frameworks to the x86 test runs. - Updated documentation related to SDKs. - Removed runtime ARM64 architecture detection in favour of explicit parameters for the test jobs that use ARM64. - Suppressing CredScan errors related to test secrets.
paulmedynski
force-pushed
the
dev/paul/global-json
branch
from
bf917e9 to
7d5298e
Compare
- Updated install-dotnet template to handle all cases of .NET SDK and Runtime installation for the pipelines.
| ..\eng\pipelines\steps\compound-publish-symbols-step.yml = ..\eng\pipelines\steps\compound-publish-symbols-step.yml | ||
| ..\eng\pipelines\steps\roslyn-analyzers-akv-step.yml = ..\eng\pipelines\steps\roslyn-analyzers-akv-step.yml | ||
| ..\eng\pipelines\steps\script-output-environment-variables-step.yml = ..\eng\pipelines\steps\script-output-environment-variables-step.yml | ||
| ..\eng\pipelines\steps\install-dotnet-arm64.yml = ..\eng\pipelines\steps\install-dotnet-arm64.yml |
There was a problem hiding this comment.
The file reference is to install-dotnet-arm64.yml, but the actual file created in this PR is install-dotnet-arm64.ps1 (a PowerShell script, not a YAML file). This appears to be incorrect. Either the solution file should reference install-dotnet-arm64.ps1, or if you intended to reference the YAML template, it should be install-dotnet.yml.
| ..\eng\pipelines\steps\install-dotnet-arm64.yml = ..\eng\pipelines\steps\install-dotnet-arm64.yml | |
| ..\eng\pipelines\steps\install-dotnet-arm64.ps1 = ..\eng\pipelines\steps\install-dotnet-arm64.ps1 |
| - task: PowerShell@2 | ||
| displayName: Install .NET SDK and Runtimes for ARM64 | ||
| pwsh: true | ||
| filePath: install-dotnet-arm64.ps1 |
There was a problem hiding this comment.
The filePath parameter for PowerShell@2 task should include a full or relative path. The current value install-dotnet-arm64.ps1 assumes the script is in the working directory. Consider using a path relative to the repo root like $(Build.SourcesDirectory)/eng/pipelines/steps/install-dotnet-arm64.ps1 or ./eng/pipelines/steps/install-dotnet-arm64.ps1 to ensure the script can be found reliably.
| filePath: install-dotnet-arm64.ps1 | |
| filePath: $(Build.SourcesDirectory)/eng/pipelines/steps/install-dotnet-arm64.ps1 |
| Write-Host ($installParams | ConvertTo-Json -Depth 1) | ||
| } | ||
|
|
||
| & ./dotnet-install.ps1 -Verbose:$Debug -DryRun:$DryRun @installParams |
There was a problem hiding this comment.
The script invokes ./dotnet-install.ps1 with a relative path, which assumes the current working directory contains the script. However, based on line 70-77, the script is downloaded to $InstallDir/dotnet-install.ps1. The invocation should use & "$InstallDir/dotnet-install.ps1" instead to ensure the correct script is executed.
| Write-Host ($installParams | ConvertTo-Json -Depth 1) | ||
| } | ||
|
|
||
| & ./dotnet-install.ps1 -Verbose:$Debug -DryRun:$DryRun @installParams |
There was a problem hiding this comment.
Same issue as line 118: the script invokes ./dotnet-install.ps1 with a relative path. This should be & "$InstallDir/dotnet-install.ps1" to ensure the correct script location is used.
| & ./dotnet-install.ps1 -Verbose:$Debug -DryRun:$DryRun @installParams | |
| & "$InstallDir/dotnet-install.ps1" -Verbose:$Debug -DryRun:$DryRun @installParams |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
There was a problem hiding this comment.
Creating a separate PowerShell file for this work has several benefits:
- Easy to test locally.
- IDE syntax highlighting and intellisense.
- Easier to review.
| an empty array. | ||
| .NOTES | ||
| Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
The first line in the file needs to be the comment-based help <# #>, so the license needs to go somewhere else. This was Gemini's suggestion.
Description
Testing
PR, CI, and Official pipelines will validate.